Base64Url Id instead of byte[]#586
Conversation
Regenhardt
force-pushed
the
feature/unraw-id
branch
from
74ed708 to
61e2694
Compare
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## master #586 +/- ##
=======================================
Coverage 76.04% 76.04%
=======================================
Files 101 101
Lines 2601 2601
Branches 432 432
=======================================
Hits 1978 1978
Misses 513 513
Partials 110 110 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
I've been reviewing this, as well has reading some of the related WebAuthn discussions. like: w3c/webauthn#2119
Although this will introduce lots of "won't-compile"-errors when people upgrade to 4.0, I agree it's probably the right thing to do.
The current set of changes look good, however I think we also need to change the Id-prop on AuthenticatorAttestationRawResponse, so I will request changes.
Regenhardt
force-pushed
the
feature/unraw-id
branch
from
61e2694 to
826d448
Compare
|
Changed the This might also be the right time to make the // might be wrong to base64url encode this...
[JsonConverter(typeof(Base64UrlConverter))]
[JsonPropertyName("rawId"), Required]
public byte[] RawId { get; init; } |
RawId is decoded to the raw byte value, while Id is the same value in base64url-encoded form.
Regenhardt
force-pushed
the
feature/unraw-id
branch
from
826d448 to
74bc4f1
Compare
|
Looks like for some reason the upload artifats github action is deprecated. Should I just update it here or would your prefer it in its own branch? |
|
@Regenhardt It's own branch please! |
|
Is this ready for review again? |
|
Yes it's ready for review, although the failed pipeline has to wait for the new artifacts action to be updated. |
|
This needs #590 to fix the pipeline. |
3 participants
#513
Not sure if any of the places that now use
RawIdshould be changed to useIdinstead, likeCredentialIdbeing returned from a successful assertion. This basically just changes the model and makes everything use the RawId, so minimal breaking changes hopefully.